Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix resource group not getting updated if tags are added #1721

Merged
merged 1 commit into from
Oct 26, 2021
Merged

Fix resource group not getting updated if tags are added #1721

merged 1 commit into from
Oct 26, 2021

Conversation

karuppiah7890
Copy link
Contributor

@karuppiah7890 karuppiah7890 commented Sep 22, 2021 •
edited
Loading

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #1696

Special notes for your reviewer:

Let me know if the release note makes sense. Specifically the mention of tags and not additional tags (the field in Azure Cluster). Also, do check the TODOs in the code which contain questions and some questions are also in the issue - #1696

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

Fix resource group not getting updated if tags are added

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 22, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @karuppiah7890. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 22, 2021
@k8s-ci-robot k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Sep 22, 2021
@karuppiah7890 karuppiah7890 mentioned this pull request Sep 22, 2021
Closed
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Sep 22, 2021
@mboersma
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 22, 2021
@karuppiah7890
Copy link
Contributor Author

I'll fix the CI issues and rerun the tests 👍

@karuppiah7890 karuppiah7890 force-pushed the fix-1696 branch 2 times, most recently from 5494ae3 to 3e61686 Compare September 23, 2021 15:39
@karuppiah7890 karuppiah7890 force-pushed the fix-1696 branch 6 times, most recently from 00ec240 to f8b14fa Compare September 24, 2021 04:05
karuppiah7890
karuppiah7890 commented Oct 15, 2021
View reviewed changes
api/v1beta1/tags.go Outdated Show resolved Hide resolved
@devigned
Copy link
Contributor

/retest

CecileRobertMichon
CecileRobertMichon reviewed Oct 22, 2021
View reviewed changes
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karuppiah7890 I just tried this out locally on tilt and everything is working as expected. I really appreciate you taking the time to think through this and make the UX and code better, thank you. Please squash when you get a chance.

lgtm

@karuppiah7890
Copy link
Contributor Author

@CecileRobertMichon Cool ! Also, did you also check the scenario of upgrading from older CAPZ controller version to the version of this PR? I was thinking about the scenario where there are some existing CAPZ managed resource groups, they might still have some tag deletion issue similar to the one mentioned in #1721 (comment) , but yeah, any new resource groups that are created and managed by CAPZ controller containing this PR code, will not have any tag deletion problems

@karuppiah7890
Copy link
Contributor Author

/retest

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Oct 23, 2021 •
edited
Loading

@karuppiah7890: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cluster-api-provider-azure-apidiff 8508da9 link false /test pull-cluster-api-provider-azure-apidiff

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@devigned
Copy link
Contributor

/test pull-cluster-api-provider-azure-e2e-exp

Seems like the test framework missed provisioning and went straight to provisioned. 🤔 Too fast?? Giving it another run.

@CecileRobertMichon
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2021
@CecileRobertMichon
Copy link
Contributor

/assign @devigned

devigned
devigned approved these changes Oct 26, 2021
View reviewed changes
Copy link
Contributor

@devigned devigned left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

Great work!!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: devigned

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2021
@k8s-ci-robot k8s-ci-robot merged commit 3ad1700 into kubernetes-sigs:main Oct 26, 2021
@k8s-ci-robot k8s-ci-robot added this to the v0.5 milestone Oct 26, 2021
@karuppiah7890 karuppiah7890 deleted the fix-1696 branch October 27, 2021 10:31
@CecileRobertMichon CecileRobertMichon modified the milestones: v0.5, v1.0 Oct 28, 2021
@nawazkh nawazkh mentioned this pull request Feb 9, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devigned devigned devigned approved these changes

@alexeldeib alexeldeib Awaiting requested review from alexeldeib

@shysank shysank Awaiting requested review from shysank

@CecileRobertMichon CecileRobertMichon Awaiting requested review from CecileRobertMichon

Assignees

@devigned devigned

@CecileRobertMichon CecileRobertMichon

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/azure Issues or PRs related to azure provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.0
Development

Successfully merging this pull request may close these issues.

Resource group should get updated if tags are added
6 participants
@karuppiah7890 @k8s-ci-robot @mboersma @CecileRobertMichon @devigned @shysank